✨ Adds new condition to VirtualMachine to the expose state of metadata+userdata #167
Conversation
github-actions
bot
added
testing-needed-e2e-fast
size/L
srm09
force-pushed
the
vm-condition/metadata-limit-exceeded
branch
2 times, most recently
from
36ab117 to
68a740a
Compare
srm09
force-pushed
the
vm-condition/metadata-limit-exceeded
branch
from
68a740a to
176a219
Compare
| @@ -10,6 +10,28 @@ import ( | |||
| "github.com/vmware-tanzu/vm-operator/api/v1alpha2/common" | |||
| ) | |||
|
|
|||
| const ( | |||
| // VirtualMachineMetadataReadyCondition documents the state of the metadata passed to the VirtualMachine. | |||
| VirtualMachineMetadataReadyCondition = "VirtualMachineMetadataReady" | |||
There was a problem hiding this comment.
I don't think we have a concept of "metadata" in v1a2? It is called VirtualMachineBootstrapSpec. Or maybe this is more nuanced and I am missing something?
There was a problem hiding this comment.
Atm I am just trying to replicate the reasons in v1a2 types, I can add a TODO and get back to updating the description on this one once we make the switch to the new types.
There was a problem hiding this comment.
Added a comment which states the intent of adding these and the future updates to these types.
api/v1alpha1/condition_consts.go
Outdated
| // cannot be found. | ||
| VirtualMachineMetadataNotFoundReason = "VirtualMachineMetadataNotFound" | ||
|
|
||
| // VirtualMachineMetadataInvalidReason (Severity=Error) documents that the supplied Cloud Init metadata is invalid. |
There was a problem hiding this comment.
specified Cloud Init metadata
This is slightly confusing, right? We chose to call it metadata -- but really, it is userdata that is specified by a field called vmMetadata.
I would reword this a little bit to make it clear that this is talking about the vmMetadata resource specified -- and that this is only applicable if the transport type is CloudInit. Same for the type below. What do you think?
There was a problem hiding this comment.
I understand your point, my longer term thought behind this one was to use it for all types of metadata regardless the type of transport we are aiming at. I will update the comments on this one to remove the coupling with CloudInit metadata and make it general purpose so as to use it broadly in the future.
| @@ -72,6 +72,9 @@ const ( | |||
| CloudInitGuestInfoUserdata = "guestinfo.userdata" | |||
| CloudInitGuestInfoUserdataEncoding = "guestinfo.userdata.encoding" | |||
|
|
|||
| // CloudInitGuestInfoMaxSize the maximum allowed size for cloud init metadata. | |||
| CloudInitGuestInfoMaxSize = 64 * 1024 | |||
There was a problem hiding this comment.
Curious: is there a way to fetch this at a cluster level? That way, we don't have to maintain this bespoke const in our code and worry about it falling out of sync with what is enforced at the Guest level.
If not, we should at least call out in comments that this is enforced by the infra (ESXi) (and maybe link a public KB) so readers are not wondering how we arrived at this const.
There was a problem hiding this comment.
This was mentioned in the GH issue linked to the PR, but makes sense to call it explicitly here. Added the reference link on where this size value is defined.
pkg/vmprovider/providers/vsphere/session/session_vm_customization.go
Outdated
Show resolved
Hide resolved
| }) | ||
| }) | ||
|
|
||
| Context("With cloud init metadata + userdata exceeding the maximum size", func() { |
There was a problem hiding this comment.
Is this a valid scenario though? IIRC, the limit is enforced by the size of the value of each GuestInfo key. If both of the keys individually are less than the limit, then we should be fine?
api/v1alpha1/condition_consts.go
Outdated
| VirtualMachineMetadataNotFoundReason = "VirtualMachineMetadataNotFound" | ||
|
|
||
| // VirtualMachineMetadataInvalidReason (Severity=Error) documents that the supplied Cloud Init metadata is invalid. | ||
| VirtualMachineMetadataInvalidReason = "VirtualMachineMetadataInvalid" |
There was a problem hiding this comment.
Part of me wants to reserve the "Invalid" key here because it feels too broad. It might be helpful in future when we decide to validating the specified user-data itself (e.g., that it's format is correct).
WDYT about renaming it VirtualMachineMetadataFormatInvalid (or something similar that indicates that the encode/decode fails -- because that's what we are really validating here)?
There was a problem hiding this comment.
ACK, this makes sense. Invalid sounds too broad, and we can in the future have more pointed reasons to include other error scenarios.
Signed-off-by: Sagar Muchhal <[email protected]>
srm09
force-pushed
the
vm-condition/metadata-limit-exceeded
branch
from
176a219 to
ce7874b
Compare
api/v1alpha1/condition_consts.go
Outdated
| // cannot be found. | ||
| VirtualMachineMetadataNotFoundReason = "VirtualMachineMetadataNotFound" | ||
|
|
||
| // VirtualMachineMetadataInvalidReason (Severity=Error) documents that the supplied Cloud Init metadata is invalid. |
There was a problem hiding this comment.
I understand your point, my longer term thought behind this one was to use it for all types of metadata regardless the type of transport we are aiming at. I will update the comments on this one to remove the coupling with CloudInit metadata and make it general purpose so as to use it broadly in the future.
api/v1alpha1/condition_consts.go
Outdated
| VirtualMachineMetadataNotFoundReason = "VirtualMachineMetadataNotFound" | ||
|
|
||
| // VirtualMachineMetadataInvalidReason (Severity=Error) documents that the supplied Cloud Init metadata is invalid. | ||
| VirtualMachineMetadataInvalidReason = "VirtualMachineMetadataInvalid" |
There was a problem hiding this comment.
ACK, this makes sense. Invalid sounds too broad, and we can in the future have more pointed reasons to include other error scenarios.
| @@ -10,6 +10,28 @@ import ( | |||
| "github.com/vmware-tanzu/vm-operator/api/v1alpha2/common" | |||
| ) | |||
|
|
|||
| const ( | |||
| // VirtualMachineMetadataReadyCondition documents the state of the metadata passed to the VirtualMachine. | |||
| VirtualMachineMetadataReadyCondition = "VirtualMachineMetadataReady" | |||
There was a problem hiding this comment.
Atm I am just trying to replicate the reasons in v1a2 types, I can add a TODO and get back to updating the description on this one once we make the switch to the new types.
| @@ -10,6 +10,28 @@ import ( | |||
| "github.com/vmware-tanzu/vm-operator/api/v1alpha2/common" | |||
| ) | |||
|
|
|||
| const ( | |||
| // VirtualMachineMetadataReadyCondition documents the state of the metadata passed to the VirtualMachine. | |||
| VirtualMachineMetadataReadyCondition = "VirtualMachineMetadataReady" | |||
There was a problem hiding this comment.
Added a comment which states the intent of adding these and the future updates to these types.
| @@ -72,6 +72,9 @@ const ( | |||
| CloudInitGuestInfoUserdata = "guestinfo.userdata" | |||
| CloudInitGuestInfoUserdataEncoding = "guestinfo.userdata.encoding" | |||
|
|
|||
| // CloudInitGuestInfoMaxSize the maximum allowed size for cloud init metadata. | |||
| CloudInitGuestInfoMaxSize = 64 * 1024 | |||
There was a problem hiding this comment.
This was mentioned in the GH issue linked to the PR, but makes sense to call it explicitly here. Added the reference link on where this size value is defined.
Minimum allowed line rate is |
srm09
added
the
testing-done-e2e-fast
| @@ -261,6 +301,12 @@ func customizeCloudInit( | |||
| } | |||
|
|
|||
| if err != nil { | |||
| errWithReason := err.(ErrWithReason) | |||
There was a problem hiding this comment.
This was supposed to be errWithReason := err.(*ErrWithReason)
Will try to add unit tests for this one.
What does this PR do, and why is it needed?
Adds a new condition to the
VirtualMachineCR to expose the state of the metadata+userdata.Which issue(s) is/are addressed by this PR?:
Fixes #148
Are there any special notes for your reviewer:
Please add a release note if necessary: